Conversation
📊 Test coverage report
|
| }, | ||
| { | ||
| onSuccess: async () => { | ||
| await queryClient.invalidateQueries({ |
There was a problem hiding this comment.
instead of invalidating queries here in onSuccess we could also do it in mutation's meta property (the only benefit would be that queryClient and getQueryKey would not be necessary).
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export interface DeviceOption { |
There was a problem hiding this comment.
most probably that type will come from @/api once server supprts it.
There was a problem hiding this comment.
Pull request overview
Adds a new Device selector UI feature to the React frontend and places it in the main toolbar, intended to let users choose an inference accelerator at the project level (Issue #837).
Changes:
- Introduces a
DeviceSelectorcomponent and supporting hook/types underfeatures/device-selector/. - Adds a project update mutation hook intended to persist the selected device via
PUT /api/v1/projects/{project_id}. - Renders the selector in the toolbar next to the existing Sources/Sinks controls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| application/ui/src/features/device-selector/device-selector.types.ts | Adds DeviceOption type for picker items. |
| application/ui/src/features/device-selector/device-selector.component.tsx | Adds the DeviceSelector UI (Suspense + Picker). |
| application/ui/src/features/device-selector/api/use-update-project-device.ts | Adds mutation hook for persisting device selection + invalidation. |
| application/ui/src/features/device-selector/api/use-get-devices.ts | Adds a devices hook (currently mock data). |
| application/ui/src/components/toolbar/toolbar.component.tsx | Places the selector into the toolbar layout. |
| const { data: _project } = useCurrentProject(); | ||
| const updateDevice = useUpdateProjectDevice(); | ||
|
|
||
| const items: DeviceOption[] = [AUTO_DEVICE, ...devices]; | ||
|
|
||
| // TODO: Replace with actual project device when API is ready | ||
| const selectedDevice = 'auto'; |
There was a problem hiding this comment.
useCurrentProject() result is currently unused, and selectedDevice is hard-coded to 'auto' while Picker is controlled via selectedKey. As a result, the picker selection will never visually change (it will always show Auto) and it won’t reflect the project’s persisted device value. Derive the selected key from project.device (fallback to auto) and/or keep local state that updates on selection / mutation success, and remove the unused _project alias if not needed.
| const { data: _project } = useCurrentProject(); | |
| const updateDevice = useUpdateProjectDevice(); | |
| const items: DeviceOption[] = [AUTO_DEVICE, ...devices]; | |
| // TODO: Replace with actual project device when API is ready | |
| const selectedDevice = 'auto'; | |
| const { data: project } = useCurrentProject(); | |
| const updateDevice = useUpdateProjectDevice(); | |
| const items: DeviceOption[] = [AUTO_DEVICE, ...devices]; | |
| const selectedDevice = project?.device ?? 'auto'; |
| // TODO: Replace with $api.useSuspenseQuery('get', '/api/v1/system/devices', ...) once the endpoint is available. | ||
| const MOCK_DEVICES: DeviceOption[] = [ | ||
| { id: 'cpu', name: 'CPU' }, | ||
| { id: 'xpu', name: 'Intel GPU' }, | ||
| { id: 'cuda', name: 'NVIDIA GPU' }, | ||
| ]; | ||
|
|
||
| export const useGetDevices = (): DeviceOption[] => { | ||
| return MOCK_DEVICES; |
There was a problem hiding this comment.
This hook currently returns a hard-coded MOCK_DEVICES list. The issue/PR acceptance criteria requires fetching available devices from GET /system/devices and always including an Auto option, so this should be implemented against the real endpoint (and the UI should handle loading/error states accordingly) rather than shipping a mock.
| // TODO: Replace with proper data type once the API is ready. | ||
| const mutation = $api.useMutation('put', '/api/v1/projects/{project_id}', { | ||
| meta: { | ||
| error: { notify: true }, | ||
| }, | ||
| }); | ||
|
|
||
| const updateDevice = (device: string): void => { | ||
| mutation.mutate( | ||
| { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| body: { device } as any, | ||
| params: { path: { project_id: projectId } }, | ||
| }, |
There was a problem hiding this comment.
body: { device } as any defeats the OpenAPI typing and can mask mismatches with the backend contract. Since the backend PUT /projects/{project_id} update schema supports device, prefer reusing the existing typed useUpdateProject hook (or use ProjectUpdateType directly) so the payload is type-checked and you don’t need an any cast.
| export const DeviceSelector = () => { | ||
| return ( | ||
| <Suspense fallback={<Loading size={'S'} />}> | ||
| <DeviceSelectorContent /> | ||
| </Suspense> | ||
| ); | ||
| }; | ||
|
|
||
| const DeviceSelectorContent = () => { | ||
| const devices = useGetDevices(); | ||
| const { data: _project } = useCurrentProject(); | ||
| const updateDevice = useUpdateProjectDevice(); | ||
|
|
||
| const items: DeviceOption[] = [AUTO_DEVICE, ...devices]; | ||
|
|
||
| // TODO: Replace with actual project device when API is ready | ||
| const selectedDevice = 'auto'; | ||
|
|
||
| const handleSelectionChange = (key: Key | null): void => { | ||
| const next = String(key); | ||
|
|
||
| if (key !== null && next !== selectedDevice) { | ||
| updateDevice.mutate(next); | ||
| } | ||
| }; |
There was a problem hiding this comment.
There are existing UI tests for similar toolbar pickers, but this new selector has no tests covering its core behavior (rendering Auto + device options, reflecting project.device, and issuing the PUT update + invalidating the project query on change). Adding a device-selector.component.test.tsx (MSW-mocked) would prevent regressions.
| <Picker | ||
| label={'Device'} | ||
| aria-label={'device'} | ||
| labelPosition={'side'} | ||
| labelAlign={'end'} | ||
| selectedKey={selectedDevice} | ||
| onSelectionChange={handleSelectionChange} |
There was a problem hiding this comment.
aria-label is currently set to a generic lowercase 'device'. For better screen reader output and consistency with other labels in this codebase, use a more descriptive label (e.g., "Device" or "Device selector"), or rely on the visible label prop if Picker already wires it up correctly.
Pull Request
Description
Added device selector
Closes #837
Type of Change
feat- New featurefix- Bug fixdocs- Documentationrefactor- Code refactoringtest- Testschore- MaintenanceRelated Issues
Breaking Changes
Examples
Screenshots